Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Db/gen remaining data structs #373

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Conversation

davidbloss
Copy link
Contributor

@davidbloss davidbloss commented Feb 15, 2024

Issues

Changelog

  • Update gen.go to generate all of the data structs
  • Add interfaces.go - these are GraphQL "interfaces" like Check
  • Add object.go - these are GraphQL "objects", the majority of data structs

There are a lot of new structs being added that likely won't do anything for a while.
Some generated structs are replacing existing structs.

  • Some of these are identical
  • Some are the same with extra fields
  • LOADS of testing updates, mostly GraphQL query updates
  • There are 4 structs that don't have the ,omitempty json tag (can be added) but wanted to vet those first

The 4 structs needing vetting are (all in object.go):

  • Level
  • FilterPredicate
  • ServiceDocument
  • Tool

It's a lot... 😁 Thank you ahead of time for review this! 🙏
I also need to add changie logs

  • List your changes here
  • Make a changie entry

Tophatting

task generate

@davidbloss davidbloss changed the title WIP: Db/gen remaining data structs Db/gen remaining data structs Feb 16, 2024
@davidbloss davidbloss marked this pull request as ready for review February 16, 2024 20:30
@@ -0,0 +1,3 @@
linters:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporarily added because go vet points out an issue with ServiceDocumentSource.
Right now it looks like:

# Embedded, both `IntegrationId` and `ServiceRepository` have a clashing `json:"id"` tag
type ServiceDocumentSource struct {
	IntegrationId     `graphql:"... on ApiDocIntegration"`
	ServiceRepository `graphql:"... on ServiceRepository"`
}

but it should look like

type ServiceDocumentSource struct {
	IntegrationId     IntegrationId     `graphql:"... on ApiDocIntegration"`
	ServiceRepository ServiceRepository `graphql:"... on ServiceRepository"`
}

autopilot.Equals(t, "my-big-query", result.Name)
}

func TestGetInfra(t *testing.T) {
// Arrange
testRequest := autopilot.NewTestRequest(
`query InfrastructureResourceGet($all:Boolean!$input:IdentifierInput!){account{infrastructureResource(input: $input){id,aliases,name,type @include(if: $all),providerResourceType @include(if: $all),providerData @include(if: $all){accountName,externalUrl,providerName},owner @include(if: $all){... on Team{teamAlias:alias,id}},ownerLocked @include(if: $all),data @include(if: $all),rawData @include(if: $all)}}}`,
`query InfrastructureResourceGet($all:Boolean!$input:IdentifierInput!){account{infrastructureResource(input: $input){aliases,data,href,id,integration,lastSyncedAt,name,owner{... on Team{teamAlias:alias,id}},ownerLocked,providerData{accountName,externalUrl,providerName},providerResourceType,rawData,type}}}`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is no good - the extra directive @include(if: $all) on things is needed because otherwise for a list infra query we'll be pulling back HUGE amounts of data

actions.go Outdated
@@ -100,7 +78,7 @@ func (client *Client) GetCustomAction(input string) (*CustomActionsExternalActio
"input": *NewIdentifier(input),
}
err := client.Query(&q, v, WithName("ExternalActionGet"))
if q.Account.Action.Id == "" {
if q.Account.Action.CustomActionsId.Id == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a poor UX change. Anyway we can make it like how it used to be?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

DedupId string `graphql:"dedupId" json:"dedupId"` // The deduplication ID provided to prevent duplicate deploys.
DeployNumber string `graphql:"deployNumber" json:"deployNumber"` // An identifier to keep track of the version of the deploy.
DeployUrl string `graphql:"deployUrl" json:"deployUrl"` // The url the where the deployment can be found.
DeployedAt string `graphql:"deployedAt" json:"deployedAt"` // The time the deployment happened.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be an ISO8601DateTime

ProviderName string `graphql:"providerName" json:"providerName"` // The integration name of the deploy.
ProviderType string `graphql:"providerType" json:"providerType"` // The integration type used the deploy.
ProviderUrl string `graphql:"providerUrl" json:"providerUrl"` // The url to the deploy integration.
Service string `graphql:"service" json:"service"` // The service object the deploy is attached to.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like its a service object so should be a ServiceId

@@ -1,16 +1,15 @@
package opslevel

// NOTE: test first, then replace this struct with commented out one below
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to be changed in this PR, or the next one?


// HasDocumentationCheck represents .
type HasDocumentationCheck struct {
Campaign string `graphql:"campaign" json:"campaign"` // The campaign the check belongs to.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a Campaign object so it needs to be a CampaignId type

// Language represents a language that can be assigned to a repository.
type Language struct {
Name string `graphql:"name" json:"name"` // The name of the language.
Usage float32 `graphql:"usage" json:"usage"` // The percentage of the code written in that language.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we know if Ruby is float64 or float32? Honestly if it works float64 is better in golang because the standard math lib works with float64 and you have to bring in a specialized math32 external library to support math operations on float32 OR you have to convert / cast type all the time.

Notes string `graphql:"notes" json:"notes"` // Additional information about the check.
Owner CheckOwner `graphql:"owner" json:"owner"` // The owner of the check.
Type CheckType `graphql:"type" json:"type"` // The type of check.
UpdateFrequency string `graphql:"updateFrequency" json:"updateFrequency"` // The minimum frequency of the updates.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another one that is a specialized type

Owner CheckOwner `graphql:"owner" json:"owner"` // The owner of the check.
Type CheckType `graphql:"type" json:"type"` // The type of check.
UpdateFrequency string `graphql:"updateFrequency" json:"updateFrequency"` // The minimum frequency of the updates.
UpdateRequiresComment string `graphql:"updateRequiresComment" json:"updateRequiresComment"` // Whether the check requires a comment or not.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is suppose to be a boolean 🤔

// NewRelicIntegration represents .
type NewRelicIntegration struct {
AccountKey string `graphql:"accountKey" json:"accountKey"` // The New Relic Account key for this integration.
AllowManualSyncAlertSources string `graphql:"allowManualSyncAlertSources" json:"allowManualSyncAlertSources"` // Indicates if manual alert source synchronization can be triggered.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one should also be a bool - this makes me a little scared about the generator is really not doing the right thing. Can we figure out why?

CreatedAt iso8601.Time `graphql:"createdAt" json:"createdAt"` // The time this integration was created.
Id ID `graphql:"id" json:"id"` // The unique identifier for the integration.
InstalledAt iso8601.Time `graphql:"installedAt" json:"installedAt"` // The time that this integration was successfully installed, if null, this indicates the integration was not completed installed.
LastManualSyncAlertSources string `graphql:"lastManualSyncAlertSources" json:"lastManualSyncAlertSources"` // The time that alert sources were last manually synchronized at.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another iso8601

@davidbloss davidbloss marked this pull request as draft October 11, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants